Skip to content

Add true zero-allocation X.509 certificate verification under WOLFSSL_NO_MALLOC#10821

Open
aidangarske wants to merge 5 commits into
masterfrom
feature/x509-no-malloc-verify
Open

Add true zero-allocation X.509 certificate verification under WOLFSSL_NO_MALLOC#10821
aidangarske wants to merge 5 commits into
masterfrom
feature/x509-no-malloc-verify

Conversation

@aidangarske

Copy link
Copy Markdown
Member

Makes the X.509 verify path (wc_ParseCert) truly be no malloc, so embedded/no-heap builds can validate certificate chains without dynamic memory.

Previously a strict no-heap build failed to parse any real certificate. The public-key copy (StoreKey/StoreEccKey) and the SubjectAltName list (AltNameNew/SetDNSEntry) needed the heap and returned MEMORY_E. This references the public key and alt-name strings in place in the source DER (the same contract wolfSSL already uses for subjectCN), with SAN entries held in a fixed per-cert pool (WC_ASN_MAX_ALTNAMES, default 8).

Gated by a new internal WC_ASN_NO_HEAP (WOLFSSL_NO_MALLOC && NO_WOLFSSL_MEMORY && !XMALLOC_USER && !WOLFSSL_STATIC_MEMORY): only builds with genuinely no allocator take the in-place path; builds with a real allocator (callbacks, XMALLOC_USER, static memory) keep the existing heap paths. Default malloc builds are byte-for-byte unchanged

Under no-heap, IP/RID SANs (which need a heap-synthesized string) and CertManager CA storage are fail-closed (documented limitations). Adds a heap-free parse test (cert_no_malloc_test).

@aidangarske aidangarske self-assigned this Jun 30, 2026
@aidangarske aidangarske requested a review from dgarske June 30, 2026 19:08
@aidangarske aidangarske marked this pull request as ready for review June 30, 2026 19:08
@github-actions

Copy link
Copy Markdown

retest this please

@aidangarske aidangarske requested a review from SparkiDev July 1, 2026 18:36
Comment thread wolfcrypt/src/asn.c
Comment thread wolfcrypt/src/asn.c Outdated
Copilot AI review requested due to automatic review settings July 2, 2026 16:13
@aidangarske aidangarske force-pushed the feature/x509-no-malloc-verify branch from 5b82762 to 46477a1 Compare July 2, 2026 16:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates wolfCrypt’s X.509 parsing/verification path to support a strict no-allocator configuration by referencing public keys and SubjectAltName (SAN) data in-place within the source DER, and by storing SAN list nodes in a fixed per-certificate pool when WC_ASN_NO_HEAP is active.

Changes:

  • Introduces WC_ASN_NO_HEAP gating and a fixed-size SAN pool (WC_ASN_MAX_ALTNAMES) in decoded certificate structures.
  • Updates key and SAN handling to avoid heap allocation (in-place key/SAN references; pool-backed SAN nodes) and adjusts freeing behavior accordingly.
  • Adds a unit test to exercise heap/file-free parsing and validate the in-place/no-heap invariants when enabled.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
wolfssl/wolfcrypt/asn.h Adds WC_ASN_NO_HEAP gating, SAN pool fields, and tracking metadata for SAN entry ownership.
wolfcrypt/src/asn.c Implements no-heap key/SAN storage paths and updates freeing and CA storage behavior under no-heap.
wolfcrypt/test/test.c Adds a parse test intended to validate no-heap/in-place storage behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread wolfssl/wolfcrypt/asn.h Outdated
Comment thread wolfcrypt/test/test.c Outdated
@aidangarske aidangarske force-pushed the feature/x509-no-malloc-verify branch from 46477a1 to ffba504 Compare July 2, 2026 16:25
@aidangarske aidangarske force-pushed the feature/x509-no-malloc-verify branch from ffba504 to d271031 Compare July 2, 2026 16:36
@aidangarske aidangarske requested a review from SparkiDev July 2, 2026 16:51
Comment thread wolfcrypt/src/asn.c
altNames->ridStringStored = 0;
}
#endif
#ifdef WC_ASN_NO_HEAP

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document WC_ASN_NO_HEAP at top of asn.c.

Comment thread wolfssl/wolfcrypt/asn.h Outdated
#define WC_ASN_NO_HEAP
#endif

#ifndef WC_ASN_MAX_ALTNAMES

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap this with WC_ASN_NO_HEAP so its clear this only applies to the NO malloc case.

Comment thread wolfssl/wolfcrypt/asn.h Outdated
int oidSum; /* provide oid sum for verification */
#endif
#ifdef WC_ASN_NO_HEAP
int entryStored; /* 1 = heap node to free; 0 = no-heap pool node */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using WC_BITFIELD or :1 with unsigned to save strut space.

Comment thread wolfcrypt/src/asn.c Outdated
/* No heap: borrow a pool slot; name points into the source DER. */
(void)heap;
#ifdef WOLFSSL_IP_ALT_NAME
/* No-heap path can't synthesise an ipString; reject rather than skip. */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider synthesise -> parse. Please document this limitation (also RID) at top of asn.c along with the macro

Comment thread wolfcrypt/src/asn.c Outdated
/* Store public key data length. */
cert->pubKeySize = pubKeyLen;
#ifdef WC_ASN_NO_HEAP
/* No heap: reference the key in place; source must outlive the cert. */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "source must outlive the cert"? Is this comment referring to the source pubKey pointer provided?

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skoll Multi-Scan Review

Modes: review + review-securityOverall recommendation: REQUEST_CHANGES
Findings: 7 total — 6 posted, 1 skipped
6 finding(s) posted as inline comments (see file-level comments below)

Posted findings

  • [High] [review] Unused static function build break under WC_ASN_NO_HEAP + WOLFSSL_IP_ALT_NAME/WOLFSSL_RID_ALT_NAMEwolfcrypt/src/asn.c:14289,14360,14577,14583
  • [Medium] [review] cert_no_malloc_test uses a cert whose IP SAN the no-heap path fail-closeswolfcrypt/test/test.c:26025-26027
  • [Medium] [review] New no-heap key/SAN in-place paths are under-testedwolfcrypt/test/test.c:26018-26046
  • [Low] [review+review-security] No-heap SAN name is not NUL-terminated (contract divergence from heap path)wolfcrypt/src/asn.c:14536-14542
  • [Low] [review+review-security] altNamePool embedded mid-struct enlarges stack-allocated DecodedCert/DecodedAcert under no-heapwolfssl/wolfcrypt/asn.h:1806-1811
  • [Low] [review] New SetDNSEntry call sites exceed the 80-column style limitwolfcrypt/src/asn.c:19066,19128,19274,19306,19322

Skipped findings

  • [Medium] Shared 8-slot SAN pool exhaustion and IP/RID fail-closed abort the entire certificate parse under no-heap

Review generated by Skoll

Comment thread wolfcrypt/src/asn.c
#ifdef WC_ASN_NO_HEAP
/* No heap: borrow a pool slot; name points into the source DER. */
(void)heap;
#ifdef WOLFSSL_IP_ALT_NAME

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 [High] Unused static function build break under WC_ASN_NO_HEAP + WOLFSSL_IP_ALT_NAME/WOLFSSL_RID_ALT_NAME · bug

GenerateDNSEntryIPString (guarded only by #ifdef WOLFSSL_IP_ALT_NAME, line 14284) and GenerateDNSEntryRIDString (#ifdef WOLFSSL_RID_ALT_NAME, line 14355) are static, and their ONLY call sites are lines 14577 and 14583, both of which the PR moved into the new #else (non-WC_ASN_NO_HEAP) branch of SetDNSEntry (the #ifdef WC_ASN_NO_HEAP ... #else ... #endif closes at line 14594). When WC_ASN_NO_HEAP is defined together with WOLFSSL_IP_ALT_NAME or WOLFSSL_RID_ALT_NAME, those callers are compiled out but the function definitions remain, producing defined but not used static functions. Under wolfSSL's common -Werror build jobs this -Wunused-function warning is a hard compile failure. This is not a hypothetical combination: the PR's own WC_ASN_NO_HEAP branch contains explicit #ifdef WOLFSSL_IP_ALT_NAME/#ifdef WOLFSSL_RID_ALT_NAME reject code (asn.c:14521-14531), so the combo is intended to be buildable.

Fix: Gate the two helper definitions with && !defined(WC_ASN_NO_HEAP) (e.g. #if defined(WOLFSSL_IP_ALT_NAME) && !defined(WC_ASN_NO_HEAP) and likewise for GenerateDNSEntryRIDString) so the no-heap + IP/RID-alt-name configuration compiles cleanly under -Werror. Add a CI build combining WC_ASN_NO_HEAP with WOLFSSL_IP_ALT_NAME to catch this.

Comment thread wolfcrypt/test/test.c Outdated

WOLFSSL_ENTER("cert_no_malloc_test");

InitDecodedCert(&cert, server_cert_der_2048, sizeof_server_cert_der_2048,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [Medium] cert_no_malloc_test uses a cert whose IP SAN the no-heap path fail-closes · test

The positive parse test uses server_cert_der_2048, which (per certs/server-cert.pem) carries X509v3 Subject Alternative Name: DNS:example.com, IP Address:127.0.0.1. Under WC_ASN_NO_HEAP combined with WOLFSSL_IP_ALT_NAME, the new SetDNSEntry rejects ASN_IP_TYPE with ASN_PARSE_E (asn.c:14521-14525), so ParseCert fails and cert_no_malloc_test returns that error, failing the whole wolfCrypt suite. The test therefore silently depends on WOLFSSL_IP_ALT_NAME being undefined, even though the code path it is meant to validate explicitly supports that macro. When -Werror is off this is the runtime symptom of the same combo that otherwise breaks the build.

Fix: Use a DER cert buffer that contains no iPAddress/registeredID SAN for the positive no-heap parse (or explicitly gate the WC_ASN_NO_HEAP assertions when WOLFSSL_IP_ALT_NAME/WOLFSSL_RID_ALT_NAME are defined), and add a separate negative test asserting ASN_PARSE_E for an IP-SAN cert under those macros.

Comment thread wolfcrypt/test/test.c Outdated
#if defined(USE_CERT_BUFFERS_2048) && !defined(NO_RSA)
/* Heap/file-free parse from a const DER buffer; checks in-place refs under
* WC_ASN_NO_HEAP. */
static wc_test_ret_t cert_no_malloc_test(void)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [Medium] New no-heap key/SAN in-place paths are under-tested · test

cert_no_malloc_test exercises only the pre-existing RSA in-place public-key path plus (at most) a single DNS SAN. The genuinely new code is not covered: StoreKey/StoreEccKey in-place references for Ed25519/Ed448/ECC keys (cert->pubKeyStored == 0), SAN pool nodes whose name pointer must land inside [source, source+maxIdx), the pool-exhaustion MEMORY_E path, and the IP/RID fail-closed path. The RSA-only positive test cannot detect regressions in the Store*Key ECC/Ed branches or in the SAN pool name references.

Fix: Broaden the no-heap test: walk the full cert.altNames chain and confirm every name pointer satisfies source <= name < source+maxIdx and entryStored == 0; add an ECC cert buffer to hit StoreEccKey no-heap; add a synthetic >WC_ASN_MAX_ALTNAMES case expecting MEMORY_E; cover the IP/RID fail-closed path.

Comment thread wolfcrypt/src/asn.c
ret = MEMORY_E;
dnsEntry = NULL;
}
else {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] No-heap SAN name is not NUL-terminated (contract divergence from heap path) · Security

The heap path NUL-terminates dnsEntry->name (dnsEntry_name[strLen] = '\0', line 14572), but the new WC_ASN_NO_HEAP path sets dnsEntry->name = (char*)str pointing directly into the source DER with NO terminator (the following byte is the next ASN.1 element), relying solely on len. Length-based consumers are safe: the core verification path (CheckForAltNames/MatchDomainName in src/internal.c) and the copy paths in src/x509.c use altName->len/dns->len. However, several OpenSSL-compat consumers treat entry->name as a NUL-terminated C string, e.g. X509PrintName (src/x509.c:6956 XSNPRINTF(scratch, MAX_WIDTH, "DNS:%s", entry->name), and the RFC822/URI cases at 6974/6981). Under no-heap those would read past the SAN value into adjacent DER bytes until a stray 0x00, potentially past the end of the cert buffer (out-of-bounds read). Reachability is low because those consumers require OPENSSL_EXTRA/OPENSSL_ALL, which in practice cannot be built under a genuine no-allocator configuration. Flagged because the heap path's NUL-termination is an implicit contract the new path silently breaks.

Fix: Document explicitly (in asn.h near WC_ASN_NO_HEAP and on struct DNS_entry) that under WC_ASN_NO_HEAP name is NOT NUL-terminated and only len is authoritative, mirroring the subjectCN contract. Audit entry->name string consumers (e.g. src/x509.c X509PrintName) to use %.*s with entry->len instead of %s, or compile-guard those print helpers out when WC_ASN_NO_HEAP is defined.

Comment thread wolfssl/wolfcrypt/asn.h Outdated
#endif
int version; /* cert version, 1 or 3 */
DNS_entry* altNames; /* alt names list of dns entries */
#ifdef WC_ASN_NO_HEAP

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] altNamePool embedded mid-struct enlarges stack-allocated DecodedCert/DecodedAcert under no-heap · Blast Radius

Under WC_ASN_NO_HEAP an inline DNS_entry altNamePool[WC_ASN_MAX_ALTNAMES] (default 8) plus altNamePoolUsed is embedded directly in both DecodedCert (asn.h:1806-1811) and DecodedAcert (asn.h:3230-3234). These structs are frequently stack-allocated on exactly the embedded/no-heap targets this feature serves, so this adds roughly 8 * sizeof(DNS_entry) bytes (order of a few hundred bytes) to stack usage per parse, which can matter on stack-constrained MCUs. It is the intended zero-allocation trade-off and is behind #ifdef (heap builds unaffected, ABI-safe since the block is fully compiled out in non-no-heap builds). Note the new members are inserted mid-struct (after altNames) rather than appended at the end, contrary to the project's new-members-at-the-end convention; this is only ABI-safe because the whole block is conditional.

Fix: Prefer appending the new members at the end of the struct. Document the added stack cost and that WC_ASN_MAX_ALTNAMES directly scales it, so integrators can size stacks / reduce the pool for constrained targets.

Comment thread wolfcrypt/src/asn.c Outdated

if (ret == 0) {
ret = SetDNSEntry(cert->heap, buf, (int)bufLen, ASN_OTHER_TYPE, &entry);
ret = SetDNSEntry(cert->heap, WC_DNS_POOL(cert), buf, (int)bufLen, ASN_OTHER_TYPE, &entry);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] New SetDNSEntry call sites exceed the 80-column style limit · style

Several updated SetDNSEntry(cert->heap, WC_DNS_POOL(cert), ...) call sites now exceed the wolfSSL 80-column convention (e.g. asn.c:19066 and 19128), unlike the neighboring wrapped calls. Minor formatting inconsistency introduced by threading the extra pool arguments through the macro.

Fix: Re-wrap the over-length lines to stay within 80 columns, matching the surrounding style.

Comment thread wolfcrypt/test/test.c Outdated
#if defined(USE_CERT_BUFFERS_2048) && !defined(NO_RSA)
/* Heap/file-free parse from a const DER buffer; checks in-place refs under
* WC_ASN_NO_HEAP. */
static wc_test_ret_t cert_no_malloc_test(void)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add CI to exercise cert_no_malloc_test.

  1. cert_no_malloc_test is unreachable in a real no-malloc build — the exact config it validates. It sits at the end of cert_test(), but cert_test's preamble does XMALLOC(FOURK_BUF,...) at test.c:26060, which returns NULL under WOLFSSL_NO_MALLOC, so cert_test returns early and never reaches it. It's also only called when WOLFSSL_TEST_CERT && !NO_FILESYSTEM, yet the test is deliberately heap/file-free. Recommend: hoist cert_no_malloc_test to run before the heap/filesystem preamble, or invoke it from the main runner independently. That's why I had to exercise the path with a standalone harness instead.
  2. CI gap — .github/workflows/no-malloc.yml sets -DWOLFSSL_NO_MALLOC but never -DNO_WOLFSSL_MEMORY, so WC_ASN_NO_HEAP is never compiled in CI. The new path needs both. Recommend adding a config with both macros.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants